Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[StateService] get historical state #638

Merged
merged 23 commits into from
Sep 29, 2021

Conversation

ZhangTao1596
Copy link
Collaborator

@ZhangTao1596 ZhangTao1596 commented Aug 30, 2021

Changes:
* Add GetState FindStates API to get historical storage or list storages by prefix
* Add README
@devhawk @shargon

@ZhangTao1596 ZhangTao1596 changed the title StateService: add historical state StateService: get historical state Aug 30, 2021
Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments left in GetState

@roman-khimov
Copy link
Contributor

I think returning state without proofs is not a very good thing. And to get a single KV pair we can already use getstateroot with getproof, so only problem seems to be a set of KV pairs that of course can be served by getstate but

  • accepting stateroot hash instead of block index/hash (it's trivial to get via getstateroot and it's needed for subsequent checks anyway)
  • without IsFind parameter (assuming it to always be true)
  • always returning a set of KV pairs along with boundary proofs (for the first and the last pairs)
    This way we could always not just get some data from RPC node, but actually verify it and trust this data correctness.

@devhawk
Copy link
Contributor

devhawk commented Aug 31, 2021

I think returning state without proofs is not a very good thing.

Given that we're not removing the ability to get proofs, what's the concern @roman-khimov? Are you worried about incenting developers to trust remote notes w/o proofs? Instead of a separate method for providing state w/o proof, could we have a method that returns the proof and the state, so that it can be retrieved in one round trip rather than requiring every key to require two round trips (getproof + verifyproof)

always returning a set of KV pairs along with boundary proofs (for the first and the last pairs) This way we could always not just get some data from RPC node, but actually verify it and trust this data correctness.

I love this idea. Since the data may span multiple pages, is it sufficient to return the boundary proofs for the first and last pairs on the page being returned?

@roman-khimov
Copy link
Contributor

Are you worried about incenting developers to trust remote notes w/o proofs?

Yeah, it's not the end of the world thing, but still we better return things that can be checked when we can.

is it sufficient to return the boundary proofs for the first and last pairs on the page being returned?

Yep, that should work, one can reconstruct a part of the MPT from a set of KV pairs and boundary proofs, per-page proofs work just fine for this too.

@devhawk
Copy link
Contributor

devhawk commented Aug 31, 2021

Yeah, it's not the end of the world thing, but still we better return things that can be checked when we can.

OK, so let's change the new getstate method to return both the proof and the value. I'd rather add a new method and not break the existing getproof method (though I suspect the number of users of that API at this moment is very low)

@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Sep 1, 2021

  • always returning a set of KV pairs along with boundary proofs (for the first and the last pairs)
    This way we could always not just get some data from RPC node, but actually verify it and trust this data correctness.

If you are suspicious of getstate results, you can randomly pick results using getproof to verify. getstate is a extension of getstorage. I don't think it's necessary to return both KV and proof, this makes result very large.

  • accepting stateroot hash instead of block index/hash (it's trivial to get via getstateroot and it's needed for subsequent checks anyway)

Got it.

  • without IsFind parameter (assuming it to always be true)

Now we have FindState and GetState.

@roman-khimov
Copy link
Contributor

I don't think it's necessary to return both KV and proof, this makes result very large.

You don't need to return proof for every KV, only the first and the last element in a set (page) that you return, it's not that hard and StateService has all the data anyway.

@ZhangTao1596
Copy link
Collaborator Author

only the first and the last element in a set (page) that you return,

Why proofs of the first and last can prove all results reliable?

@roman-khimov
Copy link
Contributor

Why proofs of the first and last can prove all results reliable?

Consider this simplified trie and a request for prefix 2:
s

You trust root hash and you get yellow proofs in return along with all green KV pairs. If you have all green KV pairs (that's important, they need to be sequential and there can't be any omissions in a sequence, but for prefix-based search that's exactly what we have) you can easily reconstruct this part of MPT and compare yellow paths, that will prove all of this subtrie (because you have a hash of 212 in 21 and you get 212 hash by adding 2121, etc).

@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Sep 1, 2021

You trust root hash and you get yellow proofs in return along with all green KV pairs. If you have all green KV pairs (that's important, they need to be sequential and there can't be any omissions in a sequence, but for prefix-based search that's exactly what we have) you can easily reconstruct this part of MPT and compare yellow paths, that will prove all of this subtrie (because you have a hash of 212 in 21 and you get 212 hash by adding 2121, etc).

So user can verify results only using the very first proof and the very last proof cross pages. We don't need to add proof in every page. We have getproof.

@roman-khimov
Copy link
Contributor

Then consumer of this API will have to retrieve and store all pages before he can verify the result. It's not very productive IMO and some users of this API might as well stop iterating at some point. I'd prefer proving data as it comes (in pages).

@ZhangTao1596
Copy link
Collaborator Author

I still don't like both proofs and KV in one request. We have getproof. If so, the method should be percisely FindStatesWithProof. Should we add proof in getstate?

@devhawk
Copy link
Contributor

devhawk commented Sep 1, 2021

I still don't like both proofs and KV in one request. We have getproof. If so, the method should be percisely FindStatesWithProof. Should we add proof in getstate?

I'm personally fine with having getstate not include the proof since we also have getproof/verifyproof. If we were designing in a greenfield, I'd argue for a single RPC method that returned storage item + proof, but since we have shipped the current version I think adding a getstate without proof makes the most sense.

So user can verify results only using the very first proof and the very last proof cross pages. We don't need to add proof in every page. We have getproof

Part of the point is to retrieve information in as few round trips as possible. For findstate I'd rather get the proof for the first and last element of each page than require findstate, getproof and verifyproof

devhawk
devhawk previously approved these changes Sep 1, 2021
@ZhangTao1596
Copy link
Collaborator Author

@superboyiii Hi bro. Can you test this pr?

@ZhangTao1596 ZhangTao1596 changed the title StateService: get historical state [StateService] get historical state Sep 3, 2021
@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Sep 22, 2021

Actually, one thing. If you call FindStates with an unused contract hash (like UInt160.Zero) you get an error The given key was not present in the dictionary. I was expecting an empty array of states. Throwing exceptions when there is no data doesn't seem like the right choice for state service RPC endpoints to me, but I'm interested in what @shargon and @roman-khimov think

It should be Unkown contract exception.

@ZhangTao1596 ZhangTao1596 dismissed stale reviews from shargon and devhawk via 334da52 September 22, 2021 07:08
@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Sep 22, 2021

We can return an empty array of KV pairs and proof of nonexistence (trie path up to the point where it can't go further because of missing element).

We can't. If the contract is not deployed, There is no contract id. If we really want to prove something, we can prove the contract inexistent in contract management contract.

int count = Settings.Default.MaxFindResultItems;
if (4 < _params.Count)
count = int.Parse(_params[4].AsString());
if (Settings.Default.MaxFindResultItems < count)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should be returned as an exception because it could mislead the interpretation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How? Can you describe this in detail? We have truncated in response and the results are ordered by key.

Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come ledger contract data is not included in state service?

Never mind

@roman-khimov
Copy link
Contributor

Primary reasons for that are described in #520. But we have getblock and getrawtransaction for ledger things.

devhawk
devhawk previously approved these changes Sep 23, 2021
shargon
shargon previously approved these changes Sep 27, 2021
@superboyiii
Copy link
Member

Tested Pass.

@superboyiii
Copy link
Member

@shargon Could you review again?

@ZhangTao1596
Copy link
Collaborator Author

Merge?

@superboyiii
Copy link
Member

@shargon Merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants